PHPORM-28 Add Scout engine to index into MongoDB Search #3205

GromNaN · 2024-11-13T16:35:58Z

Fix PHPORM-28

And "index" stands for a MongoDB collection with an Atlas Search index named "scout".
The collection name MUST be prefixed to prevent error of indexing in the same MongoDB Collection as the source documents.

Checklist

Add tests and ensure they pass

src/Scout/ScoutEngine.php

composer.json

tests/Scout/ScoutEngineTest.php

src/Scout/ScoutEngine.php

GromNaN · 2024-11-21T15:04:39Z

Integration tests require an atlas or local-atlas cluster to run.
I need to find a way to test the result as the order is not the same for each reindexation.

jmikola

Leaving an incomplete round of feedback. We'll need to schedule some time to actually go through this PR together if you need a meaningful review, as a lot of it is over my head.

tests/Models/SearchableModel.php

tests/Models/User.php

tests/Scout/ScoutEngineTest.php

tests/Scout/ScoutIntegrationTest.php

src/Scout/ScoutEngine.php

tests/Models/SearchableModel.php

tests/Models/User.php

src/Scout/ScoutEngine.php

GromNaN

Update:

Existing tests are not touched by the Scout tests. The service provider is registered only for the Scout tests and we use dedicated model classes.
Sorting search results by id field as the order is not constant between index creation.
CI and compose configuration fixed to make atlas search container more resilient (even if it still randomly crashes).
The mapping is dynamic (soft-delete field not added). Index definition to be made configurable in a follow-up ticket: PHPORM-268

GromNaN · 2025-01-08T18:54:15Z

.github/workflows/build-ci.yml

        name: "PHP ${{ matrix.php }} Laravel ${{ matrix.laravel }} MongoDB ${{ matrix.mongodb }} ${{ matrix.mode }}"

        strategy:
+            fail-fast: false


Tests with Atlas fail randomly (more than 1/4 of tries fail). fail-fast interrupt other jobs before they are successful, making it impossible to get all the tests successful simultaneously.

GromNaN · 2025-01-08T18:54:43Z

.github/workflows/build-ci.yml

                    until docker exec --tty mongodb mongosh 127.0.0.1:27017 --eval "db.runCommand({ ping: 1 })"; do
                      sleep 1
                    done
+                    until docker exec --tty mongodb mongosh 127.0.0.1:27017 --eval "db.createCollection('connection_test') && db.getCollection('connection_test').createSearchIndex({mappings:{dynamic: true}})"; do


Ensure mongot is ready.

src/Scout/ScoutEngine.php

docker-compose.yml

src/Scout/ScoutEngine.php

tests/Scout/ScoutEngineTest.php

tests/Scout/ScoutIntegrationTest.php

Move scout engine declaration to the main MongoDBServiceProvider

jmikola

Noted that most of my previous points were all addressed. Left a few more small things that you can address or ignore as you wish.

jmikola · 2025-01-14T19:53:52Z

docker-compose.yml

            - "27017:27017"
        healthcheck:
-            test: echo 'db.runCommand("ping").ok' | mongosh mongodb:27017 --quiet
+            test: mongosh --quiet --eval 'db.runCommand("ping").ok'


I assume this relies on mongosh defaulting `localhost:27017"

jmikola · 2025-01-14T19:54:56Z

.github/workflows/build-ci.yml

                    until docker exec --tty mongodb mongosh 127.0.0.1:27017 --eval "db.runCommand({ ping: 1 })"; do
                      sleep 1
                    done
+                    until docker exec --tty mongodb mongosh 127.0.0.1:27017 --eval "db.createCollection('connection_test') && db.getCollection('connection_test').createSearchIndex({mappings:{dynamic: true}})"; do


In docker-compose.yml you removed the host/port from the mongosh invocation. Should you do so here as well for consistency? I'm OK with either but wanted to ask.

I'll update it as part of PHPORM-287

jmikola · 2025-01-14T19:56:19Z

phpunit.xml.dist

        </testsuite>
    </testsuites>
    <php>
-        <env name="MONGODB_URI" value="mongodb://mongodb/"/>


Is the mongodb hostname a sensible hostname here? In #3205 (comment) you mentioned removing it in favor of the default of localhost:27017. I understand that you'd need to use an explicit value in this context, but I'd expect to see localhost:27017 instead of mongodb in that case.

It's the default localhost from the mongodb container, but from the other container, you need to specify the mongodb container name.

jmikola · 2025-01-14T20:04:02Z

src/Scout/ScoutEngine.php

+            return 0;
+        }
+
+        return $results[0]->__count;


This seems tightly coupled to the $addFields stage in performSearch(). Pointing that out in case you'd like to do something about that.

I assume $results[0] can be assumed to be a stdClass instance per the internal type map, but @param mixed $results in the doc block doesn't tell us much. You might be able to address that with an internal assertion even though the method signature cannot be changed.

jmikola · 2025-01-14T20:06:59Z

src/Scout/ScoutEngine.php

+
+    /**
+     * Get the total count from a raw result returned by the engine.
+     * This is an estimate if the count is larger than 1000.


A @see reference to https://www.mongodb.com/docs/atlas/atlas-search/counting/ may be helpful here. You can also mention that this pertains to the pipeline created in performSearch() since that's what ultimately determines this.

jmikola · 2025-01-14T20:07:37Z

src/Scout/ScoutEngine.php

+        // "filter" specifies conditions on exact values to match
+        // "mustNot" specifies conditions on exact values that must not match
+        // They don't contribute to the relevance score
+        // https://www.mongodb.com/docs/atlas/atlas-search/compound/#options


@GromNaN: Pointing this out in case you lost track of this and agreed with leaving a comment above.

github-advanced-security bot found potential problems Nov 13, 2024

View reviewed changes

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

GromNaN changed the title ~~PHPORM-28 Add Scout engined to index into MongoDB Search~~ PHPORM-28 Add Scout engine to index into MongoDB Search Nov 14, 2024

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from fa3e0a9 to 12f3323 Compare November 15, 2024 13:20

github-advanced-security bot found potential problems Nov 15, 2024

View reviewed changes

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

GromNaN force-pushed the PHPORM-28 branch from 12f3323 to 3a23e91 Compare November 15, 2024 14:20

GromNaN commented Nov 15, 2024

View reviewed changes

composer.json Outdated Show resolved Hide resolved

tests/Scout/ScoutEngineTest.php Outdated Show resolved Hide resolved

tests/Scout/ScoutEngineTest.php Outdated Show resolved Hide resolved

GromNaN commented Nov 15, 2024

View reviewed changes

src/Scout/ScoutEngine.php Outdated Show resolved Hide resolved

GromNaN commented Nov 15, 2024

View reviewed changes

src/Scout/ScoutEngine.php Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Nov 15, 2024

View reviewed changes

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from 13ff6ec to 4647cf0 Compare November 20, 2024 14:44

GromNaN marked this pull request as ready for review November 21, 2024 15:03

GromNaN requested a review from a team as a code owner November 21, 2024 15:03

GromNaN requested a review from jmikola November 21, 2024 15:03

jmikola reviewed Nov 21, 2024

View reviewed changes

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from 1b39753 to 95343b1 Compare November 23, 2024 09:57

jmikola reviewed Nov 27, 2024

View reviewed changes

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from ee6edd2 to 3d030ba Compare December 2, 2024 11:40

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from 69fe000 to 831fa21 Compare December 17, 2024 18:55

github-advanced-security bot found potential problems Dec 17, 2024

View reviewed changes

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from 7a0344c to c628969 Compare January 7, 2025 22:03

github-actions bot added the github label Jan 8, 2025

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from 9b74d41 to a5739e0 Compare January 8, 2025 16:17

GromNaN requested a review from alcaeus January 8, 2025 18:50

GromNaN commented Jan 8, 2025

View reviewed changes

GromNaN requested a review from jmikola January 9, 2025 09:42

jmikola reviewed Jan 10, 2025

View reviewed changes

GromNaN added 3 commits January 14, 2025 14:11

PHPORM-28 Add Scout engined to index into MongoDB Search

2eb8cf2

Move scout engine declaration to the main MongoDBServiceProvider

Update default configuration

22f40a0

Set typemap on cursor, only root._id is mapped

bd218ac

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from a6e2923 to 5c2c2e8 Compare January 14, 2025 18:43

GromNaN added 2 commits January 14, 2025 20:19

Factorize map/lazyMap into performMap

1ae5af1

Update tests

b22192b

GromNaN force-pushed the PHPORM-28 branch from 5c2c2e8 to b22192b Compare January 14, 2025 19:34

GromNaN requested a review from jmikola January 14, 2025 19:34

jmikola approved these changes Jan 14, 2025

View reviewed changes

GromNaN added 3 commits January 14, 2025 21:13

Update tests for map/lazyMap

1ffcc19

Improve comments

aabec4d

Explicit $setOnInsert not necessary

cc31266

GromNaN merged commit 19ed55e into mongodb:5.x Jan 14, 2025
32 checks passed

GromNaN deleted the PHPORM-28 branch January 14, 2025 21:13

GromNaN mentioned this pull request Jan 15, 2025

PHPORM-287 Split Atlas Search CI workflow #3245

Merged

GromNaN mentioned this pull request Feb 17, 2025

PHPORM-296 Enable support for Scout v10 #3280

Merged

PHPORM-28 Add Scout engine to index into MongoDB Search #3205

PHPORM-28 Add Scout engine to index into MongoDB Search #3205

Uh oh!

Conversation

GromNaN commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GromNaN commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN commented Nov 13, 2024 •

edited

Loading

GromNaN commented Nov 21, 2024 •

edited

Loading